[ISSUE #50] Feature add rocketmq exporter to nameserver - #51

I think it better to init the NAMESRV_ADDR env in nameservice_controller.go

No test changes detected alongside source modifications. Consider adding tests to cover the changes.

RocketmqExporter.Env is tagged json:"env" (no omitempty), meaning it is a required field in JSON serialization. This will cause deserialization errors or validation failures when users omit the env field. It should be json:"env,omitempty" to be consistent with the other optional fields and match the intent of an optional exporter config.

The resource default logic checks requirements.Limits.Memory().IsZero() && requirements.Requests.Memory().IsZero(), but if the user specifies CPU limits/requests without memory (or vice versa), the entire requirements struct is replaced with defaults, silently discarding the user's CPU configuration. The check should be done per-resource and only fill in missing individual values, not replace the whole struct.

Calling requirements.Limits.Memory() on a nil ResourceList (when Limits is not set at all) will panic. If nameService.Spec.Exporter.Resources.Limits is nil, .Memory() returns a zero quantity via the ResourceList getter, but requirements.Limits itself being nil means the map access is safe in Go — however requirements.Requests being nil is the same case. This is safe in current Go k8s API, but the dual-nil check should be made explicit (check len(requirements.Limits) == 0 && len(requirements.Requests) == 0) to make intent clear and guard against future regressions.

The Exporter field uses a value type (RocketmqExporter) rather than a pointer (*RocketmqExporter). Combined with omitempty, a zero-value struct won't be omitted in JSON because omitempty only omits zero values for scalar types and pointers in Go. Users who don't specify an exporter will still have an empty struct serialized, and the Enabled bool defaulting to false is the only guard. Using *RocketmqExporter would make the omission semantics correct and allow distinguishing 'not set' from 'set but disabled'.

The CRD schema defines exporter as type: object with no properties specified. This means Kubernetes will not validate any of the exporter subfields (enabled, image, env, resources, etc.). The schema should be expanded with proper property definitions to enable server-side validation and prevent misconfigured exporter specs from being accepted silently.

The example file uses image: miaolinjie/rocketmq-exporter:latest (a personal DockerHub repo) while the README example and build script reference apacherocketmq/rocketmq-exporter. Using a personal/unofficial image in example manifests is a supply chain concern and will confuse users. The example should reference the official Apache image or the image built by the provided Dockerfile.

The exporter sidecar uses NAMESRV_ADDR: 127.0.0.1:9876 (localhost). This only works correctly when the nameserver pod uses hostNetwork: true (which this example does set). However, the documentation does not make this dependency explicit. If a user enables the exporter without hostNetwork, the exporter will fail to reach the nameserver. This constraint should be documented or enforced/validated in the controller.

When nameService.Spec.Exporter.Enabled is toggled from true to false on an existing StatefulSet, the reconciler will not remove the exporter container from the pod template because the existing StatefulSet update path is not shown to handle container list diffing. Verify that the StatefulSet update logic compares and removes containers when the exporter is disabled, otherwise the sidecar will persist until the StatefulSet is deleted and recreated.

No test coverage is added for createRocketMQExporterContainer or the exporter injection logic in statefulSetForNameService. Given the resource defaulting logic has edge cases (partial resources, nil maps), unit tests should cover: exporter disabled, exporter enabled with full resources, exporter enabled with no resources (defaults applied), and exporter enabled with partial resources.

The RocketmqExporter struct has no Annotations field, but the example YAML (rocketmq_v1alpha1_rocketmq_exporter_cluster.yaml) includes annotations under the exporter spec (for prometheus scraping). These annotations will be silently dropped during deserialization since the type doesn't define them. Either add an Annotations map[string]string field to RocketmqExporter and propagate them to the pod template metadata, or remove them from the example.

The exporter field is defined as type: object with no sub-schema properties. This means Kubernetes will accept any arbitrary data under exporter without validation. The full sub-schema (enabled, image, imagePullPolicy, env, resources) should be declared here to match the Go type RocketmqExporter and provide proper CRD validation at admission time.

The Env field is missing omitempty in its JSON tag (json:"env"), making it effectively required for deserialization. However, the CRD schema does not enforce this. If a user enables the exporter without specifying env, the controller may encounter unexpected behavior. Add omitempty if env is optional, or add CRD validation if it is required.

The resource-defaults logic only falls back to defaults when both Limits.Memory() and Requests.Memory() are zero. If a user provides only CPU resources (or only one of requests/limits for memory), the defaults are skipped and the user-provided partial spec is used as-is, potentially leaving memory unconstrained. Consider checking all four resource dimensions independently.

No container port is defined on the exporter sidecar. The exporter listens on port 5557 (per application.yml), and the example YAML uses prometheus.io/port: "5557" annotations. Adding Ports: []corev1.ContainerPort{{ContainerPort: 5557, Name: "metrics"}} improves discoverability and enables Service-based scraping, not just annotation-based pod scraping.

No readiness or liveness probe is configured for the exporter sidecar. If the exporter process hangs or crashes, Kubernetes will not detect the failure or restart the container. Add at minimum an HTTP readiness probe against the metrics endpoint (e.g., GET /metrics on port 5557).

Several packages installed here (openssl, busybox-extras, git) appear unnecessary for running the exporter JAR at runtime. git is only needed for the build step — consider a multi-stage Dockerfile to keep the final image minimal and reduce the attack surface.

The checkVersion function has inverted return codes. It returns 1 (failure) when the version IS valid, and falls through to exit 2 when invalid. The caller if [ $? = 0 ] checks for success (0), which is never returned. This means every valid version (e.g., 4.5.0) triggers the error message and exit 2. The return should be return 0 on match, or the caller should check $? != 0.

The example uses a personal Docker Hub image (miaolinjie/rocketmq-exporter:latest) instead of the official apacherocketmq/rocketmq-exporter registry used elsewhere in this project. This should be updated to the official image before merge, and :latest should be replaced with a pinned version tag for reproducibility.

The comment on line 349 says 'Set Broker instance as the owner and controller' but the code sets the NameService as the owner. This is a pre-existing misleading comment, but since this PR touches this area, consider correcting it to 'Set NameService instance as the owner and controller'.

No test coverage for the new exporter sidecar injection logic. The createRocketMQExporterContainer function and the conditional injection path in statefulSetForNameService should have unit tests covering: (1) exporter enabled with full spec, (2) exporter enabled with empty/default resources, (3) exporter disabled, (4) custom imagePullPolicy vs empty.

The CRD declares exporter as an unstructured object. On clusters that prune unknown CRD fields, enabled/image/env/resources will be removed, so reconciliation always sees Enabled=false. Define the nested schema (or explicitly preserve unknown fields).

Exporter annotations from the example cannot work: RocketmqExporter has no Annotations field and the controller never copies them to the Pod template. Prometheus discovery configured in the example will not select these pods.

Providing CPU-only resource settings causes this branch to replace the entire ResourceRequirements with defaults, silently discarding the user's CPU values. Default missing resource keys individually, or only default when both resource lists are empty.

rocketmqVersion is hard-coded to V4_3_2, so the ROCKETMQ_VERSION environment variable supplied by the example is ignored. This can make exporter commands incompatible with the deployed RocketMQ version.

java is not exec'd, so PID 1 is bash and SIGTERM may not reach the JVM during Pod termination. Use exec java so the exporter can shut down gracefully.

The image build clones the exporter default branch without pinning a revision; the version build argument is not declared or used. Version-tagged images are therefore non-reproducible and may contain an incompatible exporter release.

The new Exporter field contains slices, pointers, and resource maps, but no regenerated deepcopy implementation is included. Existing generated DeepCopy methods will shallow-copy these fields, violating Kubernetes API object copy semantics.